Repository navigation
fix(relinker): agree on the syscall number between the two constant-load paths - #10439
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head e8fd90f merged onto upstream/main aeeaefa (2026-10-24); the merge is clean, and so are merges onto the current main 50aa179 and together with #10479 and #10537.
Environment: Windows 11 Home 10.0.26200, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, Python 3.13.9, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release. My machine was under heavy unrelated CPU load, so I report targeted tests only; the relinker matrix is green on the PR.
Results
- Merged tree:
syscall_number,sce_code_gaps,guest_syscall_trap,sdk_mman_syscallpass. Red/green: withSyscallScanner.cppreverted and the PR's tests kept,syscall_numberfails with('sign-extended', 'did not report syscall number 18446744073709551615'); restored, it passes. With #10479 and #10537 merged on top,syscall_numberandsce_code_gaps(including #10537's new cases) still pass. The C++syscall_scanneris Linux-only in CI and was not run here; the Python test carries the same byte sequences. - Encodings checked with
objdump -M intel:48 c7 c0 ff ff ff ff=mov rax, 0xffffffffffffffff(sign-extended),48 31 c0=xor rax, rax,66 31 c0=xor ax, ax,45 31 c0=xor r8d, r8d,44 31 c0=xor eax, r8d. Every form the PR accepts does set rax to the reported constant; I found no over-acceptance. check_conventions.pypasses;pr_overlap.pylists only #8042 as not compared (one unrelated line, and #8042 no longer merges againstmain).
Should fix (text)
- "Every form the decoded walk newly recognizes was already recognized by the byte window, so which conversions are refused or skipped is unchanged" does not hold. Outside the unwind functions a gap pattern is skipped only when the decoded walk returns no number (
SyscallScanner.cpp:302-313), and the decoded walk steps over F2/F3 prefixes that the byte window (MovesOtherRegisterThanRax,ConstantRaxSetEndingAt) cannot. Withtest_sce_code_gaps.executableand--skip-sce-module --registry:48 B8 09 00 00 00 00 00 00 00 F2 49 89 CA 0F 05 C3exits 0 onmain("Skipped 1 syscall byte pattern(s) ... 0x21e") and exits 2 on this branch ("Forbidden syscall instruction at code offset 0x21e: syscall number 9");31 C0 F3 0F 05 C3likewise flips from skipped (0x212) to refused with number 0; the unprefixed48 B8 imm64; 49 89 CA; 0F 05and the48 C7 C0forms are refused on both. The change is stricter, not looser, which is fine, but the body should say that gap patterns with an F2/F3-prefixedmov r10, rcxor a prefixedsyscallafter a48 B8 imm64or31/33 C0load are now refused wheremainskipped them.
Nits
- Body: "the sign-extended value no longer truncates through
std::optional's converting constructor" misdescribesmain:ConstantRaxLoadreturned astd::uint32_tread without sign-extension (SyscallScanner.cpp:131-139onmain), and the only optional conversion (number = ConstantSyscallNumber(...)intooptional<uint64_t>, line 304) widened. The widened return type is what lets the sign-extended value through; nothing truncated before. kExtendedSourceZeroedStub/ "stub that zeroes eax from r8d" (SyscallScannerTests.cpp:124and:207, andextended-source-zeroedin the Python test):44 31 C0isxor eax, r8d(31 /r isXOR r/m32, r32; REX.R extends the source), eax becomeseax ^ r8d; the expected "no number" is right, the name is not.- The
33 C0arm is accepted but never tested:48 33 C0(0) and a negative control such as44 33 C0=xor r8d, eax(no number) would pin it.
Notes
- The decoded path still rejects some legal constant loads:
40 31 C0(REX with no bits),66 48 31 C0(REX.W overrides 66),4C C7 C0 imm32and44 B8 imm32(REX.R is ignored for these); consequence is only a missing number in the message, and compilers do not emit them. - Cross-PR: #10537 replaces exactly the byte-window
66 31 C0arm this body says is left alone, and it reports number 0 for44 31 C0and41 33 C0, where this PR's decoded path (correctly) reports none; after both merge the two paths disagree on those bytes. I have left a note on #10537. Whichever of the two lands second should restate the resulting byte-window behaviour in its body.
Not checked: the macOS counts (231/231); the C++ syscall_scanner; a full suite; two consecutive REX bytes (48 48 31 C0 is reported as 0 by the PR; I did not verify the hardware semantics).
I found no blocking issue: the encodings are right, the counter-check holds, and the fix does what #10422 asks. The one thing to settle before merge is the body's claim about unchanged refuse/skip outcomes. For disclosure: I have open relinker PRs (#10564, #10402, #10401, #10403, #10390, #10391); none touches SyscallScanner.cpp.
Written with AI assistance (Claude Code). I ran the builds, tests and fixture checks above on my Windows 11 machine.
| return value; | ||
| } | ||
|
|
||
| if ((!hasRex || rex == 0x48) && length - opcode == 2 && (bytes[opcode] == 0x31 || bytes[opcode] == 0x33) && bytes[opcode + 1] == 0xC0 |
There was a problem hiding this comment.
This arm and the 48 B8 arm above change refuse/skip outcomes outside the unwind functions, contrary to the body: a gap pattern is skipped only when the decoded walk has no number (lines 302-313), and the decoded walk steps over F2/F3 prefixes the byte window cannot. 31 C0 F3 0F 05 C3 and 48 B8 09 00 00 00 00 00 00 00 F2 49 89 CA 0F 05 C3 (test_sce_code_gaps.executable, --skip-sce-module --registry) are skipped on main and refused with numbers 0 and 9 on this branch. Stricter, so fine, but worth stating in the body.
There was a problem hiding this comment.
Confirmed on both sides: with the test_sce_code_gaps fixture, 31 C0 F3 0F 05 C3 and 48 B8 09 00 00 00 00 00 00 00 F2 49 89 CA 0F 05 C3 are skipped on main's relinker (exit 0, one skipped pattern) and refused with numbers 0 and 9 on the branch. My subset argument held only for unprefixed patterns, because the byte window anchors at the syscall's opcode byte and an F2/F3 prefix displaces it. The body now states the stricter outcome instead of claiming it is unchanged, and both patterns are pinned as prefixed-zeroed and prefixed-mov-rax-imm64 in LOADED_NUMBERS (with 0x3b loads, refused as 59).
| RequireNoNumber(binary, kClobberedStub, "stub whose intervening instruction writes rax"); | ||
| RequireNoNumber(binary, kWordZeroedStub, "stub that zeroes only ax"); | ||
| RequireNoNumber(binary, kExtendedRegisterZeroedStub, "stub that zeroes r8d"); | ||
| RequireNoNumber(binary, kExtendedSourceZeroedStub, "stub that zeroes eax from r8d"); |
There was a problem hiding this comment.
Nit: 44 31 C0 is xor eax, r8d (31 /r = XOR r/m32, r32, REX.R extends the source), so eax is not zeroed; the expectation is right, the name and description are not. kExtendedSourceXorStub / "stub that xors eax with r8d" (and extended-source-xor in the Python test) would match the bytes.
There was a problem hiding this comment.
Renamed to kExtendedSourceXorStub / "stub that xors eax with r8d" and extended-source-xor in the Python test. 45 31 C0 (xor r8d, r8d) keeps the zeroed name, which matches its semantics.
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on exact head e8fd90f4e0533d6b3a760336f9957644b700842a, considering the current #10537 head 1f6c936b36446eeadc0c1e0f8bd37a4de8c65aa0. #10537 expands the byte-window recognizer, so the two paths now disagree on additional valid register encodings relevant to #10422:
42 31 C0isxor eax,eax(REX.X does not extend either register field). The byte window now reports 0, butConstantRaxLoadrejects every REX value except0x48, so the decoded path reports no syscall number.47 31 C0isxor r8d,r8d. The byte window skips this write and can continue to a precedingmov eax,imm32; the decoded backward walk only skips49 89 CA, so it stops at this unrelated write and reports no number instead of the loaded value.
This reintroduces the same path-dependent diagnostic the issue is meant to remove. Please align the decoded path with the prefix-aware byte recognizer (and add differential cases covering both unwind-backed and gap/fallback paths), or coordinate an alternative that makes both paths report the same value. My review of #10537's current build ran ctest --test-dir build -R '^(sce_code_gaps|syscall_number)$' --output-on-failure: 2/2 passed, but those tests do not compare the same encodings across both paths.
|
Measured MarcZX10's two cases and their neighbours on e8fd90f merged with #10537's 1f6c936 onto
Both cases hold, but #10537 does not create the disagreement: on e8fd90f alone every one of these rows already differs, because A fix that needs nothing from #10537: let Written with AI assistance (Claude Code). I ran the builds and checks above on my Windows 11 machine. |
e8fd90f to
ad6fa74
Compare
| return value; | ||
| } | ||
|
|
||
| if ((!hasRex || rex == 0x48) && length - opcode == 2 && (bytes[opcode] == 0x31 || bytes[opcode] == 0x33) && bytes[opcode + 1] == 0xC0 |
There was a problem hiding this comment.
I reproduced the remaining path disagreement on this exact head with the locally built relinker. 42 31 C0 0F 05 C3 is refused without a number in the decoded fixture, but the same bytes in an SCE gap are refused as syscall number 0. 42 sets only REX.X; 31 C0 still zeroes EAX, so the decoded path should report 0 too. A second gap fixture, B8 3B 00 00 00 47 31 C0 0F 05 C3, reports number 0 even though 47 31 C0 zeroes R8D and leaves the earlier EAX value 59 unchanged; the decoded walk instead stops at that unrelated write. These exact cases are not in the passing current tests. Please make the decoded walk agree with the byte path for these prefixes and add differential fixtures that run the same encodings through both paths.
There was a problem hiding this comment.
Both cases are fixed at 0832fe9, following the direction in the measurements below: the backward walk now steps over every register-to-register mov/xor that MovesOtherRegisterThanRax accepts (the predicate the byte window already uses; 49 89 CA is one of them), ConstantRaxLoad takes the two-byte xor when (rex & 0x05) == 0 and b8 imm32 when (rex & 0x09) == 0. Measured on the branch: decoded 42 31 C0 reports 0; the gap fixture mov eax,59; 47 31 C0 reports 59 (0 on the previous head); 44 b8 3b reports 59; the 20-byte SDK mprotect tail reports 74. Differential coverage: rex-x-zeroed and rex-r-load are in LOADED_NUMBERS on the gap path, and test_syscall_number.py mirrors the measured sequences on the decoded path (extended-index-xor, load-then-extended-zeroed/xor/move/to-r9, prefix-aware-load, mprotect-tail), with 66 48 31 c0, 41 b8 imm32 and mov eax,imm32; 41 b0 imm8 as no-number controls. The partial low writes and 66 48 31 c0 are named in the body as the remaining gap-only numbers. Full relinker suites pass on arm64 and x86-64 Rosetta; the remaining mov eax,imm32; 47/45 31 C0 gap rows depend on the byte-window side and agree once #10537's prefix-aware recognizer lands.
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ad6fa74e4ac99bafe2e33605360d51ee75ba12bc. The head delta from e8fd90f4 only renames an XOR test; the path disagreement relevant to #10422 remains. I built this exact head with GCC 12.2 in a relinker-only Release configuration. ctest -R '^(sce_code_gaps|syscall_number|syscall_scanner)$' --output-on-failure passed 3/3, but two extra exact-head fixtures reproduced the uncovered bug: decoded 42 31 C0 has no number while the SCE-gap scan reports 0; a gap fixture with mov eax,59; 47 31 C0 reports 0 though the XOR only writes R8D and should preserve 59. git diff --check upstream/main...HEAD and the exact-head conventions check pass. Hosted relinker matrix and ASan pass; product jobs are skipped. Please resolve the decoded/byte-path mismatch and add differential coverage before merge.
ad6fa74 to
0832fe9
Compare
|
Followed your analysis exactly at 0832fe9: |
ahmadmuj
left a comment
There was a problem hiding this comment.
Reviewed head 0832fe91b merged onto main 0119103fa (clean; still clean on 79f061ba5, which touches none of these files), and that merge plus #10537 1f6c936b. Linux: WSL2 Ubuntu 24.04, GCC 13.3, relinker-only Release.
What I ran
ctest -R '^(sce_code_gaps|syscall_number|syscall_scanner)$': 3/3 pass on the merge and on the merge with #10537. Withmain'sSyscallScanner.cppand your tests kept, all 3 fail (mov rax, -1gives 4294967295,prefixed-zeroedis skipped). So the C++syscall_scannernow has a Linux red/green too.- MarcZX10's two cases are fixed: decoded
42 31 c0gives 0, and the gapmov eax,59; 47 31 c0gives 59. - roboterspinne's rows, each through the decoded fixture, a gap, and the unwind function. With #10537 the decoded and gap answers match on every row except
66 48 31 c0andmov eax,59; 41 b0 05. Those are the gap-only cases the body names.
Should fix: the new step-over accepts instructions that write eax
ConstantSyscallNumber now calls MovesOtherRegisterThanRax(bytes, match.Length) on decoded instructions of any length (SyscallScanner.cpp:194). That helper was written for 2- or 3-byte raw windows. For any other length it sets rex to 0 and takes the last two bytes as opcode and ModRM (:161-167). So any 4+ byte instruction whose tail looks like 89/8B/31/33 + a mod-11 byte gets stepped over, even when it writes eax.
Example: b8 3b 00 00 00 8b 44 8b f8 0f 05 c3. objdump gives mov eax,0x3b; mov eax,DWORD PTR [rbx+rcx*4-0x8]; syscall. The tail 8b f8 reads as mov edi, eax.
main: no number in the decoded fixture and inside the function; the gap is skipped.- this PR: syscall number 59 on all three paths.
Same with 8b 44 33 f8 (mov eax,[rbx+rsi-8]), 05 00 00 31 c9 (add eax,0xc9310000) and 66 b8 89 c1 (mov ax,0xc189). The last one also creates a new disagreement in a gap. There main's byte window gives the right 49545, but the decoded walk now runs first and names 59. #10537 doesn't change any of these.
Here's what I tried locally: step over only when the instruction is just (REX) + opcode + ModRM, and pass the helper the slice that starts at the REX byte:
const std::size_t opcode = Codegen::DecodedInstruction{bytes, match.Length}.OpcodeOffset();
const std::size_t begin = opcode > 0 && (bytes[opcode - 1] & 0xF0) == 0x40 ? opcode - 1 : opcode;
if (match.Length - opcode != 2 || !MovesOtherRegisterThanRax(bytes + begin, match.Length - begin))
return std::nullopt;With that, the 3 tests still pass and the four sequences above get main's answers back (no number, skipped, 49545 in the gap). Every roboterspinne row gives the same answer as on your head. f2 49 89 ca still steps over, but now because REX.B makes it r10. Today it passes only because the dropped REX leaves rm 2 (rdx). A no-number case like b8 3b 00 00 00 8b 44 8b f8 0f 05 c3 would pin this.
Nits
kExtendedLoadStubis41 B8, which ismov r8d. Its description says "loads r9d".- Body: "steps over every register-to-register
mov/xorwhose REX, opcode and ModRM bytesMovesOtherRegisterThanRaxaccepts" holds only for 2- and 3-byte instructions right now (see above).
Checked and fine: every ConstantRaxLoad form against the SDM encodings. 48 C7 C0 sign-extends. B8 with W=0 and B=0 zero-extends. 31/33 C0 with R=0 and B=0, without 66, zeroes rax. A REX before a legacy prefix is ignored. I found no over-acceptance there.
Not checked: Windows and macOS, the full suite, real binaries.
|
The over-acceptance ahmadmuj reports comes from my suggestion. I proposed letting I reproduced it on Windows (this head on I applied ahmadmuj's snippet unchanged. All of those inputs get These two rows fail on this head and pass with the snippet, in case they help: # test_syscall_number.py, no-number list
LOAD_THEN_ADD_ENDING_IN_MOVE = bytes.fromhex("b83b00000005000089c1") + b"\x0f\x05\xc3"
# test_sce_code_gaps.py, LOADED_NUMBERS
("low-word-ending-in-move", bytes.fromhex("b8 3b 00 00 00 66 b8 89 c1 0f 05 c3"), 0x219, 0xC189),The second row pins the gap case where One correction to my earlier table: Not run: Written with AI assistance (Claude Code). I ran the builds and checks above on my Windows 11 machine. |
0832fe9 to
0f45390
Compare
|
Your fix is applied verbatim at 0f45390, with both nits corrected (the description now says the stub loads r8d, and the body states the walk steps over a mov/xor consisting only of a REX prefix, an opcode and a ModRM byte). Reproduced all your cases on the previous head and confirmed main's answers are back on this one: |
roboterspinne
left a comment
There was a problem hiding this comment.
@KSWORK2001 Thanks, checked 0f45390 merged onto main 204c674 (2026-10-26); the merge is clean. Windows 11, GCC 15.2, relinker-only Release.
- Full
ctest: 306 of 307 pass;patch_advisor_unitfails the same way on plainmain, andlong_pathsis skipped.syscall_number,sce_code_gaps,sdk_mman_syscall,guest_syscall_trapandinput_magicpass. - Counter-check against the new tests:
- The previous head's
SyscallScanner.cppfailssyscall_numberatload-then-memory-loadandsce_code_gapsatlow-word-ending-in-move; both report 59. main's fails atsign-extendedandprefixed-zeroed.
- The previous head's
- The decoys from the last round get
main's answers back: no number, or a skipped gap, and 49545 for66 b8 89 c1in a gap. Aftermov eax, 59,89 c1,f2 49 89 ca,f2 49 89 c8and66 89 c1report 59, and the mprotect tail reports 74. - The hosted Build passed on Linux, macOS and Windows.
- It still merges cleanly with #10537. With #10479 there is still one conflict in
test_syscall_number.py, where both add constants afterPREFIXED_CLOBBERED; whichever lands second keeps both blocks.
That settles the points from my review and my follow-up; I found nothing else to change.
Written with AI assistance (Claude Code). I ran the checks above on my Windows 11 machine.
|
Thank you for the thorough verification — the counter-checks against the previous head and plain main, and the merge checks against #10537 and #10479, are exactly what this needed. The #10479 conflict is the expected additive one in test_syscall_number.py; whichever lands second keeps both constant blocks. Awaiting the remaining re-reviews. |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the decoded and byte-window paths against each other on the encodings raised in review ( |
What
The syscall scan names the FreeBSD syscall number in its refusal message through two paths:
ConstantSyscallNumberwalks the decoded instructions before the pattern throughConstantRaxLoad, andConstantSyscallNumberInBytes/ConstantRaxSetEndingAtscans the raw bytes back from the pattern outside the unwind functions. The two disagreed (#10422): the decoded walk reportedmov rax, imm32(48 C7 C0 imm32) without the sign-extension the register receives, and recognized neithermov rax, imm64(48 B8 imm64) norxor eax, eax-style zeroing (31/33 C0), while the byte window recognized all of these with sign-extension. The same load therefore named a different number depending on where it sat, andmov rax, -1; syscallreported 4294967295 where rax holds0xFFFFFFFFFFFFFFFF.ConstantRaxLoadnow recognizes the byte window's forms with the same extension:48 C7 C0 imm32sign-extended,48 B8 imm64,B8 imm32zero-extended, and31/33 C0zeroing. The REX bits that redirect the written register exclude a form (REX.Bmoves theB8register to r8-r15,REX.Rextends the xor source,REX.Bthe xor destination), whileREX.Xextends no register and is accepted (42 31 C0isxor eax, eax); an operand-size prefix also excludes the xor forms, since66 31 C0clears only AX. The backward walk now steps over a register-to-registermov/xorthat consists only of a REX prefix, an opcode and a ModRM byte and writes a register other than rax —MovesOtherRegisterThanRaxis applied to the slice that starts at the REX byte, and only when the whole instruction is that slice, sof2 49 89 casteps becauseREX.Bmakes it write r10 while8b 44 8b f8(a memory load into eax) or05 31 c9 00 00(an immediate add to eax) stops the walk.ConstantRaxLoadandConstantSyscallNumberreturnstd::optional<std::uint64_t>, so the sign-extended value no longer truncates throughstd::optional's converting constructor.Refuse and skip outcomes are unchanged inside the unwind functions and in images without unwind data, where both states end in a refusal either way. Outside the unwind functions the change is stricter: a pattern there is skipped when no number is established, and the byte window anchors at the syscall's opcode byte, so an
F2/F3prefix on a preceding instruction displaces that window andmainestablished no number for a prefixed pattern. The decoded walk reads through prefixes, so such patterns are now refused with their number: in thetest_sce_code_gapsfixture,31 C0 F3 0F 05 C3and48 B8 3B 00 00 00 00 00 00 00 F2 49 89 CA 0F 05 C3are skipped onmainand refused here with numbers 0 and 59. Both cases are pinned asprefixed-zeroedandprefixed-mov-rax-imm64inLOADED_NUMBERS. Refusing is the conservative direction: a real syscall with an established number no longer slips through a gap.With this change the decoded walk answers the same as the prefix-aware byte window of #10537 for the encodings measured in review, including
42 31 C0(0),44 B8 imm32(the immediate),mov eax,imm32; 47 31 C0/45 31 C0/45 89 C0/4D 89 C1(the immediate), and the 20-byte tail of the SDK inlinemprotectstub (74);rex-x-zeroed,rex-r-loadandlow-word-ending-in-movepin three of those on both paths inLOADED_NUMBERS, and the rest are mirrored intest_syscall_number.py(which runs the decoded walk without unwind data), with the over-acceptance controlsmov eax,imm32; 8b 44 8b f8andmov eax,imm32; 05 imm32reporting no number. Three numbers remain that only the byte window reports: the partial rax writes it folds (b0,66 b8, aslow-word-after-zeroingandlow-word-ending-in-moveshow),66 48 31 C0, where the operand-size prefix still excludes the xor althoughREX.Wmakes itxor rax, rax, and a41 B0 imm8(a write to r8b) or any other r8-r15 byte write after a load, which the decoded walk conservatively stops at rather than recognizing as a write to another register. Onmain's unprefixed byte window,mov eax,imm32; 47/45 31 C0still answers 0 by reading only the last two bytes; #10537's prefix-aware recognizer corrects that side.Tested
Relinker-only build on Apple silicon (macOS 27.0.1, AppleClang 21),
main999e612599against this branch. The new cases inSyscallScannerTests.cppandtest_syscall_number.pyfail onmain(mov rax, -1reports 4294967295,mov rax, imm64andxor eax, eaxreport no number,42 31 C0reports no number) and pass on the branch, including the negative controls (66 31 C0,66 48 31 C0,45 31 C0,44 31 C0,41 B8 imm32,mov eax,imm32; 41 B0 imm8, and the over-acceptance controlsmov eax,imm32; 8b 44 8b f8,mov eax,imm32; 05 imm32report no number). The two prefixed gap patterns above were run againstmain's relinker (skipped, exit 0) and the branch's (refused with 0 and 59);mov eax,59; 47 31 C0was reproduced reporting 0 on the previous head and reports 59 now; decoded42 31 C0reports 0; the over-acceptance casesmov eax,59; 8b 44 8b f8andmov eax,59; 05 imm32were reproduced reporting 59 on the previous head and report no number now, and the gapmov eax,59; 66 b8 89 c1was reproduced reporting 59 on the previous head and reports 49545 now.SyscallScannerTests.cppruns on the Linux CI host; it was also compiled and run directly here against the branch and againstmain's relinker to record the failure. Full suite: 231/231 on the native arm64 build and 231/231 on the x86-64 Rosetta build (CMAKE_OSX_ARCHITECTURES=x86_64), before the change onmainand with it.check_conventions.py --base origin/mainpasses;pr_overlap.pyreports one shared file with #8042, whoseSyscallScanner.cppchange is a single unrelated line (an instruction-scanner construction flag).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)